build: make install.py python 3 compatiable #25583
Conversation
This patch replaces usage of `filter` in such a way that it will be compatible with Python 3. Also, this patch replaces the usage of `map` to do a side-effect work with normal `for` loop.
nodejs-github-bot
added
the
build
refack
added
the
python
| # so we walk its source directory instead... | ||
| for dirname, subdirs, basenames in os.walk('deps/npm', topdown=True): | ||
| subdirs[:] = filter('test'.__ne__, subdirs) # skip test suites | ||
| subdirs[:] = [subdir for subdir in subdirs if subdir != 'test'] |
There was a problem hiding this comment.
@thefourtheye could you put a TODO here. I actually think we should not mess with internal npm directories at all.
There was a problem hiding this comment.
@cclauss I believe it is intentional. We are just ignoring the test directories and we do not want the os.walk to traverse them. If we drop [:], it will not change the actual list of directories to be traversed.
Quoting os.walk,
When topdown is True, the caller can modify the dirnames list in-place (perhaps using del or slice assignment), and walk() will only recurse into the subdirectories whose names remain in dirnames; this can be used to prune the search, impose a specific order of visiting, or even to inform walk() about directories the caller creates or renames before it resumes walk() again.
There was a problem hiding this comment.
@refack The comment above the for loop explains why it is done. Isn't it a good reason to do this?
There was a problem hiding this comment.
The comment above the
forloop explains why it is done.
IIUC the comment explains why we walk, not why the output is filtered ¯\(ツ)/¯, IMHO npm should be a black-box and we should install it as is. But sorry for the off-topic comment.
There was a problem hiding this comment.
I wrote that comment. Perhaps I should've added "...and filter what we don't need" but that was kind of self-evident when there was still a call to filter(). :)
There was a problem hiding this comment.
@refack We wouldn't want to install the unit tests in npm, right?
There was a problem hiding this comment.
Since ATM the process for vendoring npm is to extract the release tarball, personally I'd say we should install the whole tree as a black box (So it's equivalent to npm i -g npm).
But IMO we should follow up that discussion in a separate PR.
There was a problem hiding this comment.
@refack Sure, yeah. We might want to hear from other collaborators as well.
|
@bricss Instead of a 👍 could you please consider doing a Review changes because that will give project maintainers more confidence that you has read through these code changes and believe that they improve the state of the project? Anyone can review any GitHub pull request:
Thanks for taking the time to go through these step which should help project maintainers. |
refack
added
the
author ready
|
Landed in 907ff0a. |
This patch replaces usage of `filter` in such a way that it will be compatible with Python 3. Also, this patch replaces the usage of `map` to do a side-effect work with normal `for` loop. PR-URL: #25583 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Denys Otrishko <[email protected]>
This patch replaces usage of `filter` in such a way that it will be compatible with Python 3. Also, this patch replaces the usage of `map` to do a side-effect work with normal `for` loop. PR-URL: #25583 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Denys Otrishko <[email protected]>
7 participants
This patch replaces usage of
filterin such a way that it will becompatible with Python 3. Also, this patch replaces the usage of
mapto do a side-effect work with normal
forloop.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passescc @nodejs/python @cclauss